Master nova follow ups by mujtaba1747 · Pull Request #6051 · aws/sagemaker-python-sdk

mujtaba1747 · 2026-07-17T23:45:02Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* Feat: Add show_metrics() and stream_logs() for monitoring training jobs * Feat: show_metrics() and stream_logs() --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

) * show_metrics() Enhancement: Display MLFlow metrics for OSS models * Update unit tests * Address code comments --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

* feat(serve): add opt-in model source tag-based resource reuse Add reuse_resources to ModelBuilder.build/deploy and BedrockModelBuilder.deploy. On a hit, discover an existing resource by the model-source tag and return it instead of creating a duplicate (warn, do not raise). Honored per call. - New sagemaker/serve/model_reuse.py: tag helpers + service-client discovery - Consolidate Nova manifest/checkpoint reading into sagemaker/core/training/utils.py - SageMaker: build() skips Model creation on reuse (sets built_model to the existing Model); deploy() reuses the endpoint after validating env vars/image/ instance type (PrimaryContainer with Containers[0] fallback for Nova) - Reuse gates are skipped for inference-component builds/deploys so IC create/update (via _deploy_for_ic) is never silently intercepted - Bedrock: reuse custom model + active deployment; response includes modelArn - Reuse discovery uses the cached session/bedrock clients - Support raw S3 URI model input via model_metadata BASE_MODEL_NAME - Unit tests + notebook examples * feat(serve): support Nova inference-component deployment and harden reuse Route Nova model-customization deploys through the shared single-inference -component path when a ResourceRequirements inference_config is supplied, so each Nova checkpoint (full-rank or LoRA-merged) is hosted as one inference component referencing the built Model. Nova without an inference_config keeps the direct model-on-variant path. - Broaden _is_nova_model to identify Nova from a package-less source (raw S3 checkpoint or trainer) via base_model_name, in addition to the model package recipe/hub-content name. - Set EnableNetworkIsolation on the IC endpoint config to match the built Model (always True for Nova), fixing CreateInferenceComponent rejection on mismatched network isolation. - Guard model-package-dependent logic (restricted-package path, PEFT/recipe metadata, lineage tracking) so package-less Nova checkpoints deploy cleanly. - Apply accumulated tags (including the model-source reuse tag) to endpoints created on the shared IC path so they remain discoverable. - build(reuse_resources=True) only short-circuits when the backing Model can be resolved; IC endpoints and stale/deleted configs fall through and build a real Model, preventing a None built_model on later IC deploys. - deploy() warns that reuse_resources has no effect for inference-component deployments, which manage their own reuse by component name. - Surface both the manifest.json and output.tar.gz errors when Nova checkpoint URI resolution fails, instead of masking the primary failure. Add unit tests covering the Nova IC path (routing, network isolation, IC spec) and the model-on-variant fallback. * fix(core): resolve Nova checkpoint manifest across all three output layouts Nova training jobs write their checkpoint manifest to different locations depending on the training platform: HyperPod: <output>/<job>/manifest.json Serverless: <output>/<job>/output/output/manifest.json Serverful: <output>/<job>/output/output.tar.gz (manifest inside) resolve_nova_checkpoint_uri previously only tried the serverless manifest path and the serverful tar.gz, so HyperPod jobs (manifest directly under the job directory) failed to resolve. Add build_nova_hyperpod_manifest_s3_uri and try all three layouts in turn, aggregating every failure into the raised error so the real cause is not masked by the last attempt's message. Add unit tests for the HyperPod builder and for resolution from the HyperPod and serverless layouts. * feat(serve): Model-tag reuse, IC-deploy guard, and instance_type fix Simplify reuse discovery by tagging SageMaker Models (not just endpoints) with the model-source identifier, so build(reuse_resources=True) can find and skip recreating an existing Model directly — no IC-state dependency. - Tag non-Nova Models at build time with the model-source tag (matching the Nova path's existing behavior). Both Nova and OSS Models are now discoverable by tag. - Add _find_reusable_model: build(reuse_resources=True) searches Models by source tag, skipping Model creation on a hit. Also discovers the endpoint for deploy() to reuse later. - Simplify _get_model_for_endpoint back to variant-only lookup (returns None for IC endpoints). No longer needs IC-spec resolution since the Model is found directly by tag. - _reused_endpoint_matches_config returns True for IC endpoints (can't read container config from variant; Model was already matched by tag). - deploy() with reuse_resources=True on an IC deploy logs a warning that the flag has no effect (ICs manage reuse by endpoint_name + IC name). - Fix deploy() to set self.instance_type from the caller's explicit value before calling _deploy_model_customization, preventing recipe-resolved defaults from overriding the user's intent. - Add model-source tag assertion to the existing OSS deploy integ test.

codecov · 2026-07-18T00:09:01Z

Codecov Report

❌ Patch coverage is 72.83091% with 429 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.84%. Comparing base (b8b4518) to head (c001081).
⚠️ Report is 36 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6051      +/-   ##
==========================================
+ Coverage   69.64%   69.84%   +0.19%     
==========================================
  Files         548      552       +4     
  Lines       65542    66975    +1433     
==========================================
+ Hits        45649    46776    +1127     
- Misses      19893    20199     +306

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

* feat(train): add dry_run=True to train() Add dry_run parameter to all trainers (SFT, DPO, RLVR, RLAIF). When dry_run=True: - All existing validation runs inline (IAM role, hyperparameters, recipe constraints, infrastructure availability) - Returns None without submitting a job or consuming compute - Raises with clear error message on validation failure Additionally, validate_data_path_exists() is called unconditionally (regardless of dry_run) before job submission to catch non-existent S3 paths or dataset ARNs early. Design follows nova-forge-sdk pattern: validation always runs as part of the normal code path, dry_run short-circuits before the actual TrainingJob.create API call. Changes: - data_utils.py: add validate_data_path_exists() utility (S3 + DataSet ARN) - base_trainer.py: add dry_run to abstract train(), _train_serverful_smtj(), and _train_hyperpod() - sft/dpo/rlvr/rlaif_trainer.py: add dry_run param, pass through to shared methods, short-circuit serverless path - Notebook examples added to SFT, DPO, RLVR, RLAIF notebooks - Unit tests added to existing test files - Integration test added * feat(evaluate): add dry_run=True to evaluate() Add dry_run parameter to BaseEvaluator.evaluate() and all subclasses (BenchMarkEvaluator, CustomScorerEvaluator, LLMAsJudgeEvaluator). When dry_run=True: - All existing validation runs (IAM role, model resolution, recipe, pipeline rendering) - Dataset S3 path / DataSet ARN validated via validate_data_path_exists() - Returns None without submitting a pipeline execution - Raises on validation failure Dataset validation runs unconditionally (not just during dry_run) for CustomScorerEvaluator and LLMAsJudgeEvaluator which accept user datasets. Changes: - base_evaluator.py: add dry_run to evaluate() signature - benchmark_evaluator.py: add dry_run, short-circuit before _start_execution() - custom_scorer_evaluator.py: add dry_run, validate dataset, short-circuit - llm_as_judge_evaluator.py: add dry_run, validate dataset, short-circuit - Notebook examples added to benchmark, custom_scorer, llm_as_judge notebooks * fix(dry_run): support DataSet objects, deduplicate ARN validation, expand coverage - validate_data_path_exists() now accepts Union[str, DataSet]; extracts .arn from DataSet objects for validation - Removed duplicate ARN validation logic; delegates to _validate_dataset_arn_exists() - _validate_dataset_arn_exists() warns on AccessDenied instead of raising (execution role may still have access) - Removed isinstance(..., str) guards in all trainers and evaluators so DataSet objects flow through validation - Added dry_run=True parameter to CPTTrainer.train() - Added dry_run=True parameter to ModelTrainer.train() - Integration test: valid_dataset fixture no longer re-creates on every run - Integration test: added nonexistent_dataset_arn and nonexistent_dataset_obj fixtures - Integration test: added TestDryRunServerful class (serverful compute path) - Unit test: added test_dataset_object_extracts_arn, test_dataset_object_not_found_raises * fix(dry_run): support all AWS partitions in DataSet ARN validation - Update ARN regex to aws(?:-[a-z]+)* to match aws-cn, aws-us-gov, aws-iso, aws-iso-b partitions - Use regex guard in validate_data_path_exists() for consistent matching - Add unit tests for each partition (standard, China, GovCloud, ISO, ISO-B) and invalid partition rejection

* [WIP] Job notifications setup * [WIP] Job notif update * Adding tests, dedupe logic, and example * Update ARN example values, clean up SM session definitions, rename func * Update trainers to include 'notifications' param and add arn regex check * Address PR comments, use botocore errors, return notification rule arn --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

…otification setup (#6065) * docs: Add show_metrics, stream_logs, and job notifications documentation * Update example job names * Move monitoring capabilities to model_customization --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

…ges (#6061)

#6075) * feat(evaluate): add dry_run and caller IAM permission validation to all evaluators - Add dry_run=True parameter to InspectAIEvaluator and MultiTurnRLEvaluator (Benchmark, CustomScorer, LLMAsJudge already had it) - Add verify_evaluation_caller_permissions() to validate the caller's identity has the pipeline-orchestration permissions before submitting - Define EVALUATION_CALLER_ACTIONS constant with the set of 22 IAM actions needed by whoever calls evaluator.evaluate() - Fix bug: LLMAsJudge InspectAI code path was missing the dry_run check - Wire dry_run through _get_aws_execution_context() for all evaluators - Add unit tests covering dry_run behavior across all evaluator types * change: move EVALUATION_CALLER_ACTIONS to iam_policies.py

* [Fix] Remove task-type from RLVR recipe, update RLVR image selection logic * Add check for RLVR/RFT before removing task_type * fix: prefer SMHP image over SMTJ fallback in _train_hyperpod In _train_hyperpod, try get_hyperpod_training_image first (native SMHP image) and only fall back to SMTJ image with SM-TJ->SM-HP tag replacement if the SMHP image is not available. Previously the order was inverted. --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

…6104) BedrockModelBuilder(model=trainer) now works for SFTTrainer, DPOTrainer, RLVRTrainer, etc. Previously only ModelTrainer and S3 URIs were supported.

…rock permission fail-open) (#6109) Two independent reuse defects surfaced during Nova SDK dogfooding, both under reuse_resources=True. Fixed together as they share the reuse code paths. --- 1. instance_type mismatch silently reused --- deploy() trusted the endpoint candidate cached by build() in _reused_endpoint_name without re-validating it. That cache is resolved at build time, before instance_type (a deploy() argument) is known, so a deploy on a different instance type reused the wrong endpoint. Separately, _reused_endpoint_matches_config skipped the instance_type check entirely for Inference Component (IC) endpoints: the IC early-return (no ModelName on the variant) returned True before reaching the check. deploy() now re-validates the cached endpoint against the requested instance_type and falls back to a fresh discovery on a miss or mismatch. _reused_endpoint_matches_config checks instance_type -- which lives on the production variant and is available for every endpoint, including IC ones -- before the IC early-return, so an instance-type mismatch is never silently reused. --- 2. Bedrock reuse_resources=True fails open on missing permission --- Reuse discovery (find_existing_bedrock_model, find_active_bedrock_deployment_for_model, find_existing_sagemaker_endpoint) swallowed every exception and returned None. When the execution role lacked a read permission (e.g. bedrock:ListTagsForResource), a denied discovery call was indistinguishable from "nothing to reuse", so reuse fell through to creating the resource -- which then failed with a confusing "ValidationException: Model with name '...' already exists" because the prior run's resource still held the deterministic name, never surfacing the real cause. A new _reraise_if_access_denied helper re-raises AccessDeniedException as a PermissionError naming the missing IAM action; all other errors still fail open (warn and return None) so transient failures like throttling do not block a deploy. Tests: 86 unit tests pass across test_model_builder.py and test_model_reuse.py, including regression guards for both fixes (IC instance-type mismatch, build-time cache re-validation, access-denied-raises, and fail-open-on-other-error). Co-authored-by: Elise Harvey <harveel@amazon.com>

github-actions · 2026-07-29T18:45:51Z

Claude Code is working…

I'll analyze this and get back to you.

View job run

github-actions · 2026-07-29T18:47:38Z

Claude finished @mujtaba1747's task in 1m 19s —— View job

Code Review — Master nova follow ups

Read the PR diff / changed files
Verify prior review findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

ℹ️ The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references. I re-verified each item against the current branch — all are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (685), _get_model_package_group_arn() (688), and _render_pipeline_definition() (717):

aws_context = self._get_aws_execution_context()

if dry_run:                       # line 680 — returns here
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached under dry_run
...
if dry_run:                       # line 722 — dead code, unreachable
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run is supposed to exercise. This contradicts the method's own docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which all gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

Fix: remove the first if dry_run: block (680–682) so control reaches the one at 722.

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns and won't catch a regression. Add mock_resolve.assert_called_once() to pin the "validation runs before returning" contract.

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

sagemaker-train/src/sagemaker/train/base_trainer.py:129-130

if notifications:
    self.notification_rule_arn = self._setup_notifications(notifications)

_setup_notifications makes live, mutating AWS calls (put_rule, put_targets, get_topic_attributes) at __init__ time. Consequences:

Merely constructing SFTTrainer(..., notifications=...) creates an EventBridge rule + targets before any train() call — surprising for a constructor and hard to test.
train(dry_run=True) can't suppress it — the rule already exists by construction, which undercuts the "no side effects" promise of dry-run.
The _setup_notifications docstring says it's "Called internally by trainer.train() after job submission," which is inconsistent with the actual __init__ call site.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting the docstring.

🟢 Robustness — `notifications.py`: exception handling by string matching

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:140,145 and :373

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", "") — and iam_role_resolver.py / data_utils.py elsewhere in this PR use error codes too. String matching on messages/type-names is fragile to botocore wording changes. Catch ClientError and switch on the error code consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:86

_fetch_smtj_logs takes streams[0]["logStreamName"] from describe_log_streams without passing orderBy/descending. The default ordering is LogStreamName, not guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True; otherwise filter to the exact prefix match. Fine for single-stream jobs, non-deterministic in general.

🟢 Performance — O(N) tag lookups in resource-reuse scans

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — an N+1 pattern with throttling risk on accounts with many models/endpoints. Not blocking (opt-in feature), but a good follow-up: use resourcegroupstaggingapi:GetResources (or SageMaker Search) to filter by the model-source tag server-side, or log when a bounded scan cap is hit.

Everything else looks correct and well-covered: the Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correctly uses iam:SimulatePrincipalPolicy + error codes and returns None when undeterminable), the read-only IAM validation design, partition handling, deterministic notification-rule hashing, and the dry-run threading through the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers. Nice breadth on the new dry-run integration tests.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

…6114) * fix(train): raise on expired credentials in show_metrics log fetch When AWS credentials expire, show_metrics() reported "No CloudWatch logs found for job '<name>'. The job may still be starting, or logs may not be available yet", sending users to debug their training job instead of refreshing credentials. _fetch_smtj_logs() and _fetch_smhp_logs() each wrapped their CloudWatch Logs call in a bare `except Exception`, logged a warning, and returned an empty list, so an ExpiredTokenException was indistinguishable from a job that genuinely has no logs yet. The empty list then became the misleading ValueError above. Design follows the existing notifications.py pattern: read the structured error code, re-raise the auth subset as PermissionError with remediation guidance, and let every other code keep its current degrade-to-empty behavior. A genuinely absent log group (ResourceNotFoundException) still returns an empty list, so a just-started job continues to raise the existing "No CloudWatch logs found" ValueError. Changes: - cloudwatch_metrics.py: add _AUTH_ERROR_CODES and _raise_if_auth_error(); apply at describe_log_streams, get_log_events, and filter_log_events; narrow `except Exception` to `except ClientError` - base_trainer.py: document PermissionError on show_metrics() - test_cloudwatch_metrics.py: 20 unit tests covering all 7 auth codes on both platforms, ResourceNotFoundException degradation, mid-pagination failure, and a regression guard for the existing "no logs found" path * change(train): move AUTH_ERROR_CODES to common_utils/constants.py Addresses review feedback: the auth error-code list was duplicated in cloudwatch_metrics.py and its unit test, so the two lists had to be kept in sync manually. Move it to the existing common_utils/constants.py as a module-level frozenset and import it in both places. Test parametrization sorts the frozenset so test IDs stay deterministic. --------- Co-authored-by: sayemkam <sayemkam@amazon.com>

github-actions · 2026-07-29T18:51:06Z

Claude finished @mujtaba1747's task in 1m 13s —— View job

Code Review — Master nova follow ups

Read changed files / verify prior findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

ℹ️ The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references. I re-verified each against the current branch — all still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

evaluate() has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (685), _get_model_package_group_arn() (688), and _render_pipeline_definition() (717):

aws_context = self._get_aws_execution_context()

if dry_run:                       # line 680 — returns here
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached under dry_run
...
if dry_run:                       # line 722 — dead code, unreachable
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which all gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

Fix: remove the first if dry_run: block (680–682) so control reaches the one at 722.

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns and won't catch a regression. Add mock_resolve.assert_called_once() to pin the "validation runs before returning" contract.

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

sagemaker-train/src/sagemaker/train/base_trainer.py:129-130

if notifications:
    self.notification_rule_arn = self._setup_notifications(notifications)

_setup_notifications makes live, mutating AWS calls (put_rule, put_targets, get_topic_attributes) at __init__ time. Consequences:

Merely constructing SFTTrainer(..., notifications=...) creates an EventBridge rule + targets before any train() call — surprising for a constructor and hard to test.
train(dry_run=True) can't suppress it — the rule already exists by construction, undercutting the "no side effects" promise of dry-run.
The _setup_notifications docstring says it's "Called internally by trainer.train() after job submission," inconsistent with the actual __init__ call site.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting the docstring.

🟢 Robustness — `notifications.py`: exception handling by string matching

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:140,145 and delete_notification_rule

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", "") — and iam_role_resolver.py / data_utils.py elsewhere in this PR use error codes too. String matching on messages/type-names is fragile to botocore wording changes. Catch ClientError and switch on the error code consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:86

_fetch_smtj_logs takes streams[0]["logStreamName"] from describe_log_streams without passing orderBy/descending. The default ordering is LogStreamName, not guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True; otherwise filter to the exact prefix match. Fine for single-stream jobs, non-deterministic in general.

🟢 Performance — O(N) tag lookups in resource-reuse scans

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — an N+1 pattern with throttling risk on accounts with many models/endpoints. Not blocking (opt-in feature), but a good follow-up: use resourcegroupstaggingapi:GetResources (or SageMaker Search) to filter by the model-source tag server-side, or log when a bounded scan cap is hit.

Everything else looks correct and well-covered: the Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correctly uses iam:SimulatePrincipalPolicy + error codes and returns None when undeterminable), the read-only IAM validation design, partition handling, deterministic notification-rule hashing, and the dry-run threading through the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers. Nice breadth on the new dry-run integration tests.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* fix: serverful instance type validations + integ tests * cleanup: add missing newlines to end of files * style(sagemaker-train): Fix missing trailing newlines * fix(train): Log SMHP enum fetch failure at debug, not warning + added unit tests --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

github-actions · 2026-07-29T21:28:50Z

Claude finished @mujtaba1747's task in 1m 17s —— View job

Code Review — Master nova follow ups

Read changed files
Verify findings against current code
Post consolidated review

ℹ️ The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references. I re-verified each against the current branch — the 🔴 bug is still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (685), _get_model_package_group_arn() (688), and _render_pipeline_definition() (717):

aws_context = self._get_aws_execution_context()

if dry_run:                       # line 680 — returns here
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached under dry_run
...
if dry_run:                       # line 722 — dead code, unreachable
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

Fix: remove the first if dry_run: block (680–682) so control reaches the one at 722.

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() to pin the "validation runs before returning" contract.

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

sagemaker-train/src/sagemaker/train/base_trainer.py:130-131

if notifications:
    self.notification_rule_arn = self._setup_notifications(notifications)

_setup_notifications makes live, mutating AWS calls (put_rule, put_targets, get_topic_attributes) at __init__ time. Consequences:

Merely constructing SFTTrainer(..., notifications=...) creates an EventBridge rule + targets before any train() call — surprising for a constructor and hard to test.
train(dry_run=True) can't suppress it — the rule already exists by construction, undercutting the "no side effects" promise of dry-run.
The _setup_notifications docstring says it's "Called internally by trainer.train() after job submission," inconsistent with the actual __init__ call site.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting the docstring.

🟢 Robustness — `notifications.py`: exception handling by string matching

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:140,145 and delete_notification_rule

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule matches on the exception type name. Meanwhile _validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", "") — and iam_role_resolver.py / data_utils.py elsewhere in this PR use error codes too. String matching on messages/type-names is fragile to botocore wording changes. Catch ClientError and switch on the error code consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:120

_fetch_smtj_logs takes streams[0]["logStreamName"] from describe_log_streams (called with only logStreamNamePrefix, no orderBy/descending). The default ordering is LogStreamName, not guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True; otherwise filter to the exact prefix match. Fine for single-stream jobs, non-deterministic in general.

🟢 Performance — O(N) tag lookups in resource-reuse scans

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — an N+1 pattern with throttling risk on accounts with many models/endpoints. Not blocking (opt-in feature), but a good follow-up: use resourcegroupstaggingapi:GetResources (or SageMaker Search) to filter by the model-source tag server-side, or log when a bounded scan cap is hit.

Everything else looks correct and well-covered: the Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correctly uses iam:SimulatePrincipalPolicy + error codes and returns None when undeterminable), the read-only IAM validation design, partition handling, deterministic notification-rule hashing, and the dry-run threading through the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* Update error message on ModelBuilder when deploying from S3 checkpoint * Update ModelBuilder to automatically find image_uri * Update import and methods * Improving logging level * fix: update job key used for show_metrics/stream_logs in MTRL

github-actions · 2026-07-30T16:16:56Z

Claude Code is working…

I'll analyze this and get back to you.

View job run

* fix(stream_logs): unify log streaming for MTRL, evaluators, and HyperPod Fixes three stream_logs() bugs identified in bug bash testing: 1. MTRL trainer stream_logs() now uses the correct log group (/aws/sagemaker/Job/AgentRFT) and polls status via Job API instead of TrainingJob API — previously hung forever showing nothing. 2. Adds stream_logs() to BaseEvaluator and EvaluationPipelineExecution with support for pipeline, MTRL eval, and HyperPod backends. 3. Patches _stream_logs_smhp() to provide user feedback instead of silently swallowing ResourceNotFoundException and empty events. Introduces LogStreamer utility (poll-once pattern) and stream_log_loop() shared helper to eliminate code duplication across all callers. * test(stream_logs): add evaluator integ tests using existing completed jobs Integration tests for evaluator.stream_logs() against completed pipeline executions in us-west-2/729646638167. Covers BenchMarkEvaluator, CustomScorerEvaluator, and LLMAsJudgeEvaluator. No new jobs launched.

github-actions · 2026-07-30T16:18:08Z

Claude finished @zhaoqizqwang's task in 1m 15s —— View job

Code Review — Master nova follow ups

Verify prior findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

ℹ️ The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references. I re-verified each against the current branch — the 🔴 bug and all notes are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (685), _get_model_package_group_arn() (688), and _render_pipeline_definition() (717):

aws_context = self._get_aws_execution_context()

if dry_run:                       # line 680 — returns here
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached under dry_run
...
if dry_run:                       # line 722 — dead code, unreachable
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

Fix: remove the first if dry_run: block (680–682) so control reaches the one at 722.

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() to pin the "validation runs before returning" contract.

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

sagemaker-train/src/sagemaker/train/base_trainer.py:131-132

# Set up notifications if configured
if notifications:
    self.notification_rule_arn = self._setup_notifications(notifications)

_setup_notifications makes live, mutating AWS calls (put_rule, put_targets, get_topic_attributes) at __init__ time. Consequences:

Merely constructing SFTTrainer(..., notifications=...) creates an EventBridge rule + targets before any train() call — surprising for a constructor and hard to test.
train(dry_run=True) can't suppress it — the rule already exists by construction, undercutting the "no side effects" promise of dry-run.
The _setup_notifications docstring says it's "Called internally by trainer.train() after job submission," inconsistent with the actual __init__ call site.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting the docstring.

🟢 Robustness — `notifications.py`: exception handling by string matching

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:139-149 and :373

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", "") — and iam_role_resolver.py / data_utils.py elsewhere in this PR use error codes too. String matching on messages/type-names is fragile to botocore wording changes. Catch ClientError and switch on the error code consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix and then takes streams[0]["logStreamName"]. The default ordering is LogStreamName, not guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True; otherwise filter to the exact prefix match. Fine for single-stream jobs, non-deterministic in general.

🟢 Performance — O(N) tag lookups in resource-reuse scans

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — an N+1 pattern with throttling risk on accounts with many models/endpoints. Not blocking (opt-in feature), but a good follow-up: use resourcegroupstaggingapi:GetResources (or SageMaker Search) to filter by the model-source tag server-side, or log when a bounded scan cap is hit.

Everything else looks correct and well-covered: the Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correctly uses iam:SimulatePrincipalPolicy + error codes and returns None when undeterminable), the read-only IAM validation design, partition handling, deterministic notification-rule hashing, and the dry-run threading through the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

ehsu3 and others added 3 commits July 9, 2026 13:20

Feat: show_metrics() and stream_logs() helper functions (#6002)

c0aead5

* Feat: Add show_metrics() and stream_logs() for monitoring training jobs * Feat: show_metrics() and stream_logs() --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

show_metrics() Enhancement: Display MLFlow metrics for OSS models (#6013

b3152fb

) * show_metrics() Enhancement: Display MLFlow metrics for OSS models * Update unit tests * Address code comments --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

mujtaba1747 temporarily deployed to auto-approve July 17, 2026 23:45 — with GitHub Actions Inactive

This was referenced Jul 18, 2026

[DO NOT MERGE] test: verify Codecov config takes effect #6054

Closed

[DO NOT MERGE] test: verify Codecov config takes effect (same-repo) #6055

Closed

fix: remove codecov after_n_builds so single-submodule PRs report coverage #6056

Merged

mujtaba1747 temporarily deployed to auto-approve July 20, 2026 17:06 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 20, 2026 17:31 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 21, 2026 15:54 — with GitHub Actions Inactive

documentation: add dry-run and resource reuse docs to existing RST pa…

58caf17

…ges (#6061)

mujtaba1747 temporarily deployed to auto-approve July 21, 2026 17:59 — with GitHub Actions Inactive

Merge branch 'master' into master-nova-follow-ups

d577e50

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 17:56 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 20:25 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 21:09 — with GitHub Actions Inactive

fix(serve): BedrockModelBuilder accepts BaseTrainer as model input (#…

579fba2

…6104) BedrockModelBuilder(model=trainer) now works for SFTTrainer, DPOTrainer, RLVRTrainer, etc. Previously only ModelTrainer and S3 URIs were supported.

mujtaba1747 temporarily deployed to auto-approve July 29, 2026 18:45 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 29, 2026 18:46 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 29, 2026 18:50 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 29, 2026 21:28 — with GitHub Actions Inactive

zhaoqizqwang temporarily deployed to auto-approve July 30, 2026 16:16 — with GitHub Actions Inactive

zhaoqizqwang temporarily deployed to auto-approve July 30, 2026 16:17 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Master nova follow ups - #6051

Master nova follow ups#6051
mujtaba1747 wants to merge 21 commits into
masterfrom
master-nova-follow-ups

mujtaba1747 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

mujtaba1747 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py: dry-run returns before the validation it advertises

🟡 Design — base_trainer.py: notifications provisioned as a constructor side-effect, not gated by dry_run

🟢 Robustness — notifications.py: exception handling by string matching

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

github-actions Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py: dry-run returns before the validation it advertises

🟡 Design — base_trainer.py: notifications provisioned as a constructor side-effect, not gated by dry_run

🟢 Robustness — notifications.py: exception handling by string matching

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

github-actions Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py: dry-run returns before the validation it advertises

🟡 Design — base_trainer.py: notifications provisioned as a constructor side-effect, not gated by dry_run

🟢 Robustness — notifications.py: exception handling by string matching

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py: dry-run returns before the validation it advertises

🟡 Design — base_trainer.py: notifications provisioned as a constructor side-effect, not gated by dry_run

🟢 Robustness — notifications.py: exception handling by string matching

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jul 18, 2026 •

edited

Loading

github-actions Bot commented Jul 29, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

🟢 Robustness — `notifications.py`: exception handling by string matching

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

github-actions Bot commented Jul 29, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

🟢 Robustness — `notifications.py`: exception handling by string matching

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

github-actions Bot commented Jul 29, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

🟢 Robustness — `notifications.py`: exception handling by string matching

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

github-actions Bot commented Jul 30, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

🟢 Robustness — `notifications.py`: exception handling by string matching

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering